Skip to content
This repository was archived by the owner on Oct 11, 2022. It is now read-only.

Conversation

@dev-drprasad
Copy link
Contributor

@dev-drprasad dev-drprasad commented May 21, 2018

Status

  • WIP
  • Ready for review
  • Needs testing

Deploy after merge (delete what needn't be deployed)

  • hyperion (frontend)
Before After
screen shot 2018-05-21 at 11 13 44 pm screen shot 2018-05-21 at 11 10 06 pm
screen shot 2018-05-21 at 11 13 28 pm screen shot 2018-05-21 at 11 10 22 pm
screen shot 2018-05-21 at 11 13 17 pm screen shot 2018-05-21 at 11 10 45 pm
screen shot 2018-05-21 at 11 36 19 pm screen shot 2018-05-21 at 11 35 41 pm

Fixes #3098

@dev-drprasad
Copy link
Contributor Author

dev-drprasad commented May 21, 2018

@brianlovin @mxstbr i have added screenshots of quoted message before and after changes. the current changes affecting both received and sent messages. i thought its looks good for sent messages also, but i need your opinion on this.

@dev-drprasad
Copy link
Contributor Author

@brianlovin @mxstbr @uberbryn if design for sent messages looks fine, i can move to ready for review

@brianlovin
Copy link
Contributor

Yeah, go ahead and do that - I think we'll wait for a final approve from @uberbryn and we can ship :)

@superbryntendo
Copy link
Contributor

Text contrast is too low, otherwise good changes. If you make the contrast ratio at least 2.5 (ideally AA), we can ship it.

@dev-drprasad
Copy link
Contributor Author

@uberbryn
text in quoted sent message has contrast ratio ~ 2.77
text in quoted received message has contrast ratio ~ 2.73
though both are below AA level

currently we are using colours from theme theme.brand.default , theme.generic.default, theme.text.alt , so i am not sure how to increase contrast, because changing theme values will affect everywhere

i am thinking of using css filter: brightness(). though only latest browsers support this
what do you think ?? is there any way to increase contrast ??

@brianlovin
Copy link
Contributor

If those are above 2.5 let's ship them; I think we'll have to do another pass down the road and potentially add some higher contrast colors below our text.alt level :)

@brianlovin brianlovin merged commit 9f51467 into withspectrum:alpha May 24, 2018
@superbryntendo
Copy link
Contributor

I misread this as changing from light bg to dark bg and adding spacing. that light bg is much higher contrast, so maybe we should just make both bgs that color. (not high priority)

@brianlovin
Copy link
Contributor

Good point @uberbryn - since they are both in a quote reply might as well be the same color

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants